-
-
Notifications
You must be signed in to change notification settings - Fork 745
Implement opCmp for Nullable objects #10762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
format as per D style
Format as per D style
Format as per D style
Format as per D style
|
Buildkite failure looks unrelated: gecko0307/dagon#106 |
Format as per D style
@PetarKirov went for a round 2 of Testing without making any changes but fails again due to same reason |
std/typecons.d
Outdated
| * Returns: | ||
| * Negative if `this < rhs`, zero if equal, positive if `this > rhs`. | ||
| */ | ||
| int opCmp(this This, Rhs)(auto ref Rhs rhs) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use typeof(this) instead of this This if you need the this pointers type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is no longer needed, you can remove this template parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int opCmp(this This, Rhs)(auto ref Rhs rhs) const | |
| int opCmp(Rhs)(auto ref Rhs rhs) const |
Co-authored-by: Richard (Rikki) Andrew Cattermole <[email protected]>
Remove extra parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DDoc comment says that this opCmp function is intended to compare two Nullable values, but the actual code allows values that are not Nullable. The code should be fixed to match the documentation.
| * Negative if `this < rhs`, zero if equal, positive if `this > rhs`. | ||
| */ | ||
| int opCmp(Rhs)(auto ref Rhs rhs) const | ||
| if (is(typeof(_value.payload < rhs.get)) && is(typeof(_value.payload > rhs.get))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test will return true for any type with a get method that returns a value comparable to the payload, even if it is not a Nullable type. The correct way to check whether Rhs is a Nullable type whose payload is comparable to this Nullable's payload is like this:
if (is(Rhs == Nullable!U, U) && is(typeof(this.get < rhs.get)))(Strictly speaking, it doesn't matter whether you use payload or get in the typeof(...) expression, but you should use the same on both sides for consistency.)
| static if (is(Rhs == Nullable)) | ||
| { | ||
| if (_isNull) | ||
| return rhs._isNull ? 0 : -1; | ||
| else if (rhs._isNull) | ||
| return 1; | ||
| else | ||
| return _value.payload < rhs._value.payload ? -1 : | ||
| _value.payload > rhs._value.payload ? 1 : 0; | ||
| } | ||
| else | ||
| { | ||
| static if (is(typeof(rhs.isNull))) | ||
| { | ||
| if (_isNull) | ||
| return rhs.isNull ? 0 : -1; | ||
| else if (rhs.isNull) | ||
| return 1; | ||
| else | ||
| return _value.payload < rhs.get ? -1 : | ||
| _value.payload > rhs.get ? 1 : 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you've fixed the template constraint, you can get rid of the static if (is(Rhs == Nullable)) statement and combine these two branches.
| else | ||
| { | ||
| return _isNull ? -1 : (_value.payload < rhs ? -1 : (_value.payload > rhs ? 1 : 0)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is unreachable and should be deleted.
|
This PR has had a broken CI for 6+ months, please reopen if you plan to continue working on this. |
Fixes : #10743
Summary
This PR adds a opCmp implementation for Nullable, enabling it to participate in standard algorithms like sort, uniq, and allowing direct comparison with raw values or other nullable-like types.
Root Cause
Previously, 'Nullable' lacked a flexible 'opCmp' method that allowed it to be compared with another 'Nullable' or 'Nullable'-like objects with '.isNull' and '.get' .
This limitation meant that generic code and algorithms relying on opCmp (e.g., sort, uniq, associative containers) could not work with Nullable values out of the box.
For example :
would fail to compile due to missing comparison support between Nullable instances .
Fix
This patch introduces a generic opCmp(this This, Rhs) function with appropriate constraints to ensure type safety and correct behavior .
The implementation handles :
Nullable vs Nullable: compares null-ness, then payload.
Nullable vs nullable-like: uses .isNull and .get.
Nullable vs raw value: directly compares to rhs.
Null is considered less than any non-null value, and two nulls are considered equal.
Example Reproducer
This now compiles and runs as expected. Previously, this would result in a compile-time error.
Notes
Unittest addition for basic comparison between null and non-null values
Verified locally using custom Phobos build and test suite.
This patch fully resolves the issue described in #10743.